Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add understand chapter for HIP Graphs #3571

Closed
wants to merge 1 commit into from

Conversation

MKKnorr
Copy link

@MKKnorr MKKnorr commented Aug 13, 2024

No description provided.

@MKKnorr MKKnorr force-pushed the understand-hip-graphs branch 8 times, most recently from 8808c52 to 96b0172 Compare August 13, 2024 13:09
@MKKnorr MKKnorr self-assigned this Aug 13, 2024
@MKKnorr MKKnorr marked this pull request as ready for review August 13, 2024 13:16
@neon60
Copy link
Contributor

neon60 commented Aug 14, 2024

@MKKnorr CI fails, please fix it.

@MKKnorr
Copy link
Author

MKKnorr commented Aug 14, 2024

@MKKnorr CI fails, please fix it.

That was just a timeout due to some downloads in the linting failing. The checks passed before. Triggering the pipeline again fixed it

Copy link

@randyh62 randyh62 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In general I think an example would be a great addition, even if it was just pseudocode.

docs/understand/hipgraph.rst Outdated Show resolved Hide resolved
docs/understand/hipgraph.rst Outdated Show resolved Hide resolved
docs/understand/hipgraph.rst Outdated Show resolved Hide resolved
docs/understand/hipgraph.rst Outdated Show resolved Hide resolved
docs/understand/hipgraph.rst Outdated Show resolved Hide resolved
docs/understand/hipgraph.rst Outdated Show resolved Hide resolved
docs/understand/hipgraph.rst Show resolved Hide resolved
@MKKnorr
Copy link
Author

MKKnorr commented Aug 19, 2024

In general I think an example would be a great addition, even if it was just pseudocode.

I agree, but that's a better fit for a how-to page. As stated in another comment, that section will most likely not be ready soon. A tutorial is also planned, on top of this documentation

@MKKnorr MKKnorr force-pushed the understand-hip-graphs branch 2 times, most recently from 90728f6 to 07dce2f Compare August 20, 2024 16:08
Copy link

@randyh62 randyh62 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just noticed this is consolidated under PR #3585

I think this PR can be closed in favor of that one.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This image could use a little green, or blue, or yellow.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree, that the colour choice was not the best, but I tried to keep it close to the existing colour schemes. I replaced it with a (hopefully) nicer colour set

@@ -146,7 +146,7 @@ For Linux developers, the link [here](https://github.com/ROCm/hip-tests/blob/dev

## HIP Graph

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it necessary to have this here? it is not much more than a reference, which shows up two or three times in the index or TOC.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree, that it's not really needed here, but the programming manual is going to be overhauled and probably incorporated into the runtime api how-to soon, as it is just an amalgamation of snippets on different HIP-topics.

HIP graph
********************************************************************************

HIP graphs are an alternative way of executing work on a GPU. It can provide

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would eliminate this first paragraph, leave the following Beta note in place, and start this Understanding HIP Graphs topic with the Graph format text (but without the header).

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done


The standard way of launching work on GPUs via streams incurs a small overhead
for each iteration of the operation involved. For kernels that perform large
operations during an iteration this overhead usually is negligible. However

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

operations during an iteration this overhead is usually negligible. However

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed

The standard way of launching work on GPUs via streams incurs a small overhead
for each iteration of the operation involved. For kernels that perform large
operations during an iteration this overhead usually is negligible. However
many workloads, including scientific simulations and AI, a kernel performs a

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

in many workloads, such as scientific simulations and AI, a kernel performs a

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed

@MKKnorr
Copy link
Author

MKKnorr commented Sep 23, 2024

Superseded by #3585

@MKKnorr MKKnorr closed this Sep 23, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants